-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core] [2/N] Implement uv processor #48486
[core] [2/N] Implement uv processor #48486
Conversation
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
.buildkite/data.rayci.yml
Outdated
@@ -29,28 +29,28 @@ steps: | |||
|
|||
# tests | |||
- label: ":database: data: arrow v9 tests" | |||
tags: | |||
tags: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linter makes all these changes, otherwise I cannot push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you split the unrelated lint changes to another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good suggestion and I thought about it also, my confusion is, when I tried to push on the master branch, prepush hook doesn't seem to work :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just checkout the changes of these files.
I think the hook is configured to fix changed files only.
the people who added the pre-commit's should have fixed all the lint errors across the entire repo first.. and then run it on CI with -a
flag..
rllib/algorithms/dreamerv3/README.md
Outdated
@@ -49,13 +49,13 @@ in combination with the following scripts and command lines in order to run RLli | |||
### [Atari100k](../../tuned_examples/dreamerv3/atari_100k.py) | |||
```shell | |||
$ cd ray/rllib/tuned_examples/dreamerv3/ | |||
$ python atari_100k.py --env ale_py:ALE/Pong-v5 | |||
$ python atari_100k.py --env ale_py:ALE/Pong-v5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prepush linter hook change, otherwise I cannot push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what prepush linter are you using? the pre-commit one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I add it to my git pre-push hook, which automatically fixes linting issues before push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR: #48557, I guess it needs to be merged before this PR, otherwise cannot update and push still. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG.
Also rebase with master to for [1/N]
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
This PR has already rebased upon first PR, which gets merged earlier today :) |
Signed-off-by: hjiang <hjiang@anyscale.com>
@@ -310,6 +311,7 @@ def __init__( | |||
_validate: bool = True, | |||
mpi: Optional[Dict] = None, | |||
image_uri: Optional[str] = None, | |||
uv: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this type annotation right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just revisited, I think both pip
and uv
type annotation is incorrect, fixed.
python/ray/tests/unit/test_uv.py
Outdated
runtime_env = TestRuntimeEnv() | ||
|
||
uv_processor = uv.UvProcessor(target_dir=target_dir, runtime_env=runtime_env) | ||
await uv_processor._run() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we are testing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole uv process only does download work, which is not expected in unit test.
For here, I simply goes though the initialization and mocks all the IO operations out.
It might sound weird, but I did uncover a few bugs during UT (i.e. pip
not correctly replaced with uv
).
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
"The 'pip' field, 'conda' field and 'uv' field of " | ||
"runtime_env cannot both be specified.\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one of 'pip' field, 'conda' field and 'uv' field of runtime_env can be specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow with another comment, revert all exposed public interfaces.
uv: Either a list of pip packages, or a python dictionary that has one fields: | ||
packages (required, List[str]): a lists of uv packages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add the doc when 'uv' is usable. With this PR, people cannot use 'uv' plugin yet.
I think changes to this file should be in the next PR when we actually implement the uv plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Updated.
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
@jjyao Finally I cleaned up all the stale usage after the refactor, could you please help me merge the PR? Thank you! |
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com> Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>
This PR does two things:
uv
and package installationuv
andpip
into multiple util files